Fix issue #31 #32

devanshj · 2019-07-21T01:48:51Z

~~The RegEx /\{[^\}]*$.*$[^\{]*\}/ looks good to me...~~
~~Heh nope, doesn't allow div { a (b c } and even div { a ((( }. One fix could be to use /\{[^\}\{]*\}/ but that would allow things like div { a }(b){ c} also...~~

~~So till I figure out a good way to fix this, consider this PR as a draft.~~
Okay so with the second commit the fix looks good to me. I have also added a bunch of tests. It did involve a little trial-and-error but I guess the tests are covering enough cases.

Also there was a minor mistake where the condition does a /$/.test(abbreviation) && /$/.test(abbreviation) to check if it has parentheses but it should be || instead.

/cc @ramya-rao-a @octref

- add a RegEx in the condition to allow `(` and `)` inside `{` and `}` - add corresponding tests

msftclas · 2019-07-21T01:49:05Z

All CLA requirements met.

devanshj · 2019-07-22T18:54:48Z

src/emmetHelper.ts

-	// Grouping in abbreviation is valid only if preceeded/succeeded with one of the symbols for nesting, sibling, repeater or climb up
-	if (!/\(.*\)[>\+\*\^]/.test(abbreviation) && !/[>\+\*\^]\(.*\)/.test(abbreviation) && /\(/.test(abbreviation) && /\)/.test(abbreviation)) {
+	// Grouping in abbreviation is valid only if it's inside a text node or preceeded/succeeded with one of the symbols for nesting, sibling, repeater or climb up
+	if ((/\(/.test(abbreviation) || /\)/.test(abbreviation)) && !/\{[^\}\{]*[\(\)]+[^\}\{]*\}(?:[>\+\*\^]|$)/.test(abbreviation) && !/\(.*\)[>\+\*\^]/.test(abbreviation) && !/[>\+\*\^]\(.*\)/.test(abbreviation)) {


If it's not obvious then the (?:[>\+\*\^]|$) part is to invalidate abbreviations like div{ a }( b ){ c }.

octref · 2019-09-25T22:39:03Z

I tried div{ foo ((( abc } and div{ a (b) c}+div{ a (( }, both doesn't expand on emmet doc. Why are the valid expansions with unbalanced parens?

octref · 2019-09-26T16:47:48Z

In the cases when you can't tell, we can be more liberal than emmet in accepting inputs (as long as it can expand).

devanshj · 2019-09-26T16:51:18Z

Sorry for the mess & spam here's a summarised answer:

"div{ foo ((( abc }" is valid because parenthesis don't mean anything between {, } and thus we shouldn't require to balance them.
require("./expand/expand-full.js").expand("div{ foo ((( abc }") successfully expands to <div> foo ((( abc </div>
The reason it won't expand on docs.emmet.io and even in VSCode (testExpand("html", "div{ foo ((( abc }", "<div> foo ((( abc </div>") fails) is because @emmetio/extract-abbreviation doesn't recognize it.

Action point:

Fix @emmetio/extract-abbreviation so that require("@emmetio/extract-abbreviation")("div{ foo ((( abc }") returns { abbreviation: "div{ foo ((( abc }" } (currently returns { abbreviation: "abc }" }
I'm not sure but I guess we should merge this PR and then fix @emmetio/extract-abbreviation because this PR alone also will fix things for balanced parenthesis case.

devanshj · 2019-09-26T16:51:48Z

In the cases when you can't tell, we can be more liberal than emmet in accepting inputs (as long as it can expand).

Yesss exactly. But now the problem is unbalanced parenthesis do expand theoretically but @emmetio/extract-abbreviation doesn't recognize them.

src/test/emmetHelper.test.ts

Co-Authored-By: Pine <octref@users.noreply.github.com>

octref · 2019-09-26T17:16:37Z

Fix @emmetio/extract-abbreviation so that require("@emmetio/extract-abbreviation")("div{ foo ((( abc }") returns { abbreviation: "div{ foo ((( abc }" } (currently returns { abbreviation: "abc }" }

Sounds good. You can send a PR to https://github.com/emmetio/extract-abbreviation#readme. Thanks a lot for looking into this!

devanshj · 2019-09-26T17:21:13Z

Sounds good. You can send a PR to emmetio/extract-abbreviation#readme. Thanks a lot for looking into this!

Yeah I'm looking into fixing it, once I fix it I'll send a PR till the time I would also file an issue there. Also the pleasure is all mine! :D

devanshj · 2019-10-01T15:32:30Z

Hi @octref so now emmet PR #571, #569 that were responsible for fixing the extraction are merged.

I tried to migrate from @emmetio/extract-abbreviation & src/expand/expand-full.js to the newer emmet@2.0.0 but looks like it isn't trivial.

Also I went through almost all issues on vscode repo that were labelled as emmet and a lot of them can be just fixed by updating to v2 since @sergeche has fixed a bunch of issues of @emmetio/extract-abbreviation in the newer emmet like #20 (fixes #45724), #17, #23 (fixes #52345), #24 (fixes #59172), maybe more.

Also I was kinda verifying if the newer version fixes some issues and it does here's the StackBlitz that demonstrates it. Afaict #63703, #80923, #59951, #71002 and probably more can be closed with the newer emmet.

So I guess it will be worth migrating to the newer emmet, and also the older one's are deprecated so vscode will have to migrate to newer one at some point to take advantage of further developments

Also let me know if I should create an issue for migration instead of having all this written here.

octref · 2019-10-01T16:26:53Z

@devanshj Wow, thanks a lot for looking into this. Good work 👍

Moving to emmet v2 makes sense. It just involves a lot of effort. I remember the reason we had expand-full.js as a single file was due to bundling issues. We can look into this again.

To start, I'd create an issue in vscode repo and link to all issues that would be fixed by upgrading to v2. I'll try to get some time in Oct/Nov to help you with it.

devanshj · 2019-10-01T17:52:55Z

@octref Thanks and welcome! 😁 And yeah I'll be down with helping with this as much as I can.

BMBurstein · 2020-02-16T09:13:18Z

Thanks to both of you. Is this still moving forward?

devanshj · 2020-02-16T11:13:24Z

@BMBurstein

Thanks to both of you.

You're welcome!

Is this still moving forward?

PR #33 was ready to merge until I realised that it doesn't have full test coverage so I had to dig deeper that's when I found out it's a whole mess. I started rewriting it, even remodeling and that's when I realized we don't need this package seperately because it's not doing much. That's when I started patching the extension itself to not use this module (ie vscode-emmet-helper). Even that is still wip (I haven't pushed) and I haven't looked into it in a long time. I'll see if I can look into it soon :)

(You can see description of PR #33 on why I'm ditching this module)

So to answer your question, it's stagnant at the moment but I will work on migrating to emmet@2.x.x once I've time for it.

Fix issue #31

11ba2c0

- add a RegEx in the condition to allow `(` and `)` inside `{` and `}` - add corresponding tests

Use more permissive RegEx for fixing #31, minor fixes & refactor

c86069b

devanshj mentioned this pull request Jul 22, 2019

Bug: Emmet expansion doesn't work when parentheses are used inside text node microsoft/vscode#77776

Closed

devanshj commented Jul 22, 2019

View reviewed changes

octref added this to the August 2019 milestone Jul 25, 2019

octref self-assigned this Jul 25, 2019

This comment has been minimized.

Sign in to view

octref approved these changes Sep 26, 2019

View reviewed changes

src/test/emmetHelper.test.ts Outdated Show resolved Hide resolved

Formatting

52b2a1d

Co-Authored-By: Pine <octref@users.noreply.github.com>

octref merged commit 075cb17 into microsoft:master Sep 26, 2019

octref modified the milestones: August 2019, September 2019 Sep 26, 2019

devanshj mentioned this pull request Nov 23, 2019

[Draft] Upgrade to emmet@2 #33

Closed

devanshj mentioned this pull request Oct 16, 2020

Update emmet #39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #31 #32

Fix issue #31 #32

devanshj commented Jul 21, 2019 •

edited

Loading

msftclas commented Jul 21, 2019 •

edited

Loading

devanshj Jul 22, 2019 •

edited

Loading

octref commented Sep 25, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

octref commented Sep 26, 2019

devanshj commented Sep 26, 2019 •

edited

Loading

devanshj commented Sep 26, 2019 •

edited

Loading

octref commented Sep 26, 2019

devanshj commented Sep 26, 2019

devanshj commented Oct 1, 2019 •

edited

Loading

octref commented Oct 1, 2019

devanshj commented Oct 1, 2019

BMBurstein commented Feb 16, 2020

devanshj commented Feb 16, 2020 •

edited

Loading

Fix issue #31 #32

Fix issue #31 #32

Conversation

devanshj commented Jul 21, 2019 • edited Loading

msftclas commented Jul 21, 2019 • edited Loading

devanshj Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

octref commented Sep 25, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

octref commented Sep 26, 2019

devanshj commented Sep 26, 2019 • edited Loading

devanshj commented Sep 26, 2019 • edited Loading

octref commented Sep 26, 2019

devanshj commented Sep 26, 2019

devanshj commented Oct 1, 2019 • edited Loading

octref commented Oct 1, 2019

devanshj commented Oct 1, 2019

BMBurstein commented Feb 16, 2020

devanshj commented Feb 16, 2020 • edited Loading

devanshj commented Jul 21, 2019 •

edited

Loading

msftclas commented Jul 21, 2019 •

edited

Loading

devanshj Jul 22, 2019 •

edited

Loading

devanshj commented Sep 26, 2019 •

edited

Loading

devanshj commented Sep 26, 2019 •

edited

Loading

devanshj commented Oct 1, 2019 •

edited

Loading

devanshj commented Feb 16, 2020 •

edited

Loading